-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(connext): request collateral for order amount #1885
Conversation
Does this also expose the connext remote balance in |
Yes, trading limits should now return the actual collateral available for receiving payments, as opposed to a max number like it used to. |
Here the discussion about adding a 3% buffer: #1845 (comment) . If you agree on adding it, let's do it @sangaman |
cb701b6
to
63294f7
Compare
I updated this PR to remove collateral requests upon depositing funds to a channel and to use the best available price for market orders for purposes of calculating inbound/outbound amounts. I also added a 5% buffer to the amount of collateral requested, up from the 3% originally discussed to better handle the cases with market orders since we optimistically calculate the entire order based off the best available price. |
Screenshot from 2020-09-14 14-23-39
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it works for limit orders:
➜ xud git:(collateral-on-order) ✗ ./bin/xucli buy 0.01 ETH/BTC 0.01
2 UNKNOWN: channel collateralization in progress, please try again in ~1 minute
➜ xud git:(collateral-on-order) ✗ ./bin/xucli buy 0.01 ETH/BTC 0.01
2 UNKNOWN: channel collateralization in progress, please try again in ~1 minute
➜ xud git:(collateral-on-order) ✗ ./bin/xucli buy 0.01 ETH/BTC 0.01
matched 0.01 ETH @ 0.01 with peer FlightMillion order 5ce90f90-f691-11ea-af6a-b103d10b44b2, attempting swap...
successfully swapped 0.01 ETH with peer order 5ce90f90-f691-11ea-af6a-b103d10b44b2
This is intentional to avoid over collateralizing the channel (we'll wait for the first collateralization to finish before attempting new ones). |
@sangaman could you also please rebase this against master? |
checked one more time, same result. |
Logs would be helpful I guess |
first comment contains xud and connext logs on screens |
Does the collateral request ever complete for you? Based on the behavior and the logs, my best guess is that Connext is just taking a very long time to grant the collateral, longer than your testing period. I'd wonder if the same testing setup would succeed in placing an order ~hours later. If xud is making the call and connext is receiving it, I'm not sure what else we can do from our end. I'm not too familiar with the process for requesting collateral from the connext node so maybe @erkarl will have more ideas here. |
63294f7
to
c46dbd0
Compare
In the screenshot above we can see that the connext client returned a response: https://user-images.githubusercontent.com/29906866/93080784-eeca8b00-f696-11ea-9a85-13a2a9318ab7.png |
I also managed to reproduce this on simnet:
Looking at connext client logs the collateralization request DID return (almost immediately), but
|
Meant to post here that I'd rebased this branch and added tests for checking inbound capacity. |
@raladev so the issue was the the node needed to be configured to allow higher collateralization amounts. ETH collateralizations on simnet should now allow up to 55 ETH. |
This makes a lot of sense, thanks for figuring it out. |
60970a1
to
df2a1fa
Compare
Evidently I had misunderstood how collateral requests work - I thought we were requesting the amount of additional collateral that we wanted when in fact we want to request the total collateral we want (including any existing collateral). I updated the PR to request the total amount of collateral we want, so in your tests above it should be 0.315 ETH which is 0.3 + 5%. I also updated the tests to account for this, including a case where we still expect to request the full amount of collateral needed + 5% even when we already have some existing collateral. |
df2a1fa
to
b4153f0
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above
b4153f0
to
e212f86
Compare
I needed to make some changes for the collateral request upon opening channels to go with requesting full collateral instead of incremental amounts. I just pushed that so it should work.
This is because of the minimum collateral request size. The minimum is 0.1 for ETH so when we already have 0.2205, we jump up to to 0.3205 for the next request (rather than 0.2415). That's so that we don't wind up making lots of small collateral requests. Is that also how you understand it @kilrau? |
e212f86
to
549c55c
Compare
Yup, correct. |
|
549c55c
to
ebc5f32
Compare
@raladev Thanks again for detecting that issue. I wasn't correctly identifying market orders by their price (which, the way we do it, isn't |
ebc5f32
to
98c1e33
Compare
Rebased to get latest USDT/DAI contract info. |
This adds a check when a new order is receiving tokens via connext that ensures we have sufficient collateral from the connext node to fulfill the order. If not, we fail the order request and request the additional collateral necessary. This involves the following changes: 1. The Connext client now tracks the inbound node collateral for each currency and refreshes this value every time it queries for channel balances and every time a collateral request completes. 2. Before placing a new order that is receiving Connext tokens we check the inbound amount against the available collateral for that currency. In the case of market orders, we use the best available matching price in the order book to calculate inbound & outbound amounts. 3. If the collateral is insufficient, the order will be rejected and a collateral request will be performed for the necessary capacity plus a 5% buffer. 4. Any other collateral checks while we're awaiting a response for a collateral request will not perform an additional collateral request so as to prevent duplicate calls. 5. Traders may repeat their order request and it will be accepted once sufficient collateral to complete the trade is acquired. 6. Collateral is only requested upon depositing funds to a Connext channel according to hardcoded collateral request minimums per currency. There are also several hardcoded collateral request minimums. If the capacity shortage is smaller than these minimums, the minimum amount will be requested instead. Closes #1845.
98c1e33
to
75078c0
Compare
I updated the PR to add the |
This adds a check when a new order is receiving tokens via connext that ensures we have sufficient collateral from the connext node to fulfill the order. If not, we fail the order request and request the additional collateral necessary. This involves the following changes:
The Connext client now tracks the inbound node collateral for each currency and refreshes this value every time it queries for channel balances and every time a collateral request completes.
Before placing a new order that is receiving Connext tokens we check the inbound amount against the available collateral for that currency. In the case of market orders, we use the best available matching price in the order book to calculate inbound & outbound amounts.
If the collateral is insufficient, the order will be rejected and a collateral request will be performed for the missing capacity.
Any other collateral checks while we're awaiting a response for a collateral request will not perform an additional collateral request so as to prevent duplicate calls.
Traders may repeat their order request and it will be accepted once sufficient collateral to complete the trade is acquired.
Collateral is no longer requested upon depositing funds to a Connext channel, instead it is only requested upon placing orders.
Closes #1845.
Note that this PR does not remove the collateralization that happens after depositing/opening a channel. I figure that can be done in a separate PR, but we may want to reconsider instead. One thing that occurred to me while working on this is that if we only issue market orders, the collateralization may never happen since it's based on requesting a fixed amount based on the price and quantity of orders we place. However, market orders don't have a price and therefore don't have a specific amount of collateral for us to request. Maybe there's a cleaner solution for dealing with this without requesting collateral after opening a channel, but I haven't thought of it yet.